Skip to content

Fix in-memory metadatabase creation in defaultMetadatabase - #508

Open
lukaskubanek wants to merge 3 commits into
pointfreeco:mainfrom
structuredpath:fix-in-memory-metadatabase
Open

Fix in-memory metadatabase creation in defaultMetadatabase#508
lukaskubanek wants to merge 3 commits into
pointfreeco:mainfrom
structuredpath:fix-in-memory-metadatabase

Conversation

@lukaskubanek

Copy link
Copy Markdown
Contributor

Motivation

The metadatabase is meant to live in the same world as the user database. When the user database is on disk, the metadatabase is created as a file next to it. When the user database is in-memory (useful for tests), the metadatabase is meant to be in-memory as well, so the whole setup stays ephemeral and isolated. For that case, URL.metadatabase(…) builds the corresponding SQLite address (file:sqlitedata_icloud?mode=memory&cache=shared).

However, defaultMetadatabase(…) converts that URL to a plain path before opening it, which cannot represent an in-memory address. The dedicated DatabaseQueue path for in-memory URLs was removed in #367 (see this discussion). The effect of this change depends on the OS version:

  • On macOS 15, url.path(percentEncoded: false) returns an empty string, so DatabasePool(path: "") crashes when activating WAL mode.
  • On macOS 26 and later, it returns "sqlitedata_icloud", which SQLite receives as an ordinary filename. Instead of a throwaway in-memory database, a real file named sqlitedata_icloud is silently created in the current working directory of the process (verified on macOS 27).

As most tests in the test suite use temporary on-disk databases, the issue doesn’t surface much. The few tests exercising in-memory user databases (in SyncEngineTests and SyncEngineValidationTests) don't observe where the metadatabase writer lands, though they do leave a stray sqlitedata_icloud file behind in the working directory.

There is another side effect when the metadatabase is attached to the user database connection via attachMetadatabase(…) in prepareDatabase. That code path handles the in-memory URL correctly, so the sync engine ends up with two different metadatabases at once. The triggers installed on the user’s tables write metadata into the attached in-memory store, while the sync engine's own writer reads and writes the on-disk file, each seeing only part of the picture. I verified this briefly on macOS 27.

Implementation

This PR restores the DatabaseQueue path for in-memory URLs in defaultMetadatabase(…) that was removed in #367, matching how attachMetadatabase(…) already handles them. The on-disk case is unchanged and continues to use DatabasePool with a regular file path.

It also adds a regression test that derives the in-memory URL via URL.metadatabase(…), exactly as SyncEngine.init does, and asserts the resulting metadatabase is actually in-memory.

The test lives in a standalone suite rather than being nested in BaseCloudKitTests, since it doesn't use any of that infrastructure, similar to UserlandTests.

Supersedes #410, which contained the same fix framed as a macOS 15 issue only.


Disclaimer: AI assistance (Claude Code with Fable model) was used to investigate this issue and prepare this PR. All changes and verifications were reviewed by me.

Comment on lines +21 to +23
// NB: SQLite reports an empty file path for in-memory databases. A non-empty path
// means the metadatabase was silently created on disk.
#expect(mainDatabaseFile == "")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// NB: SQLite reports an empty file path for in-memory databases. A non-empty path
// means the metadatabase was silently created on disk.
#expect(mainDatabaseFile == "")
// Metadatabase is in-memory
#expect(mainDatabaseFile == "")

@mbrandonw mbrandonw left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @lukaskubanek! Could you apply the one small change I made and then we can merge? Comments above #expect become part of test failure messages, so we can keep it short and succinct, and no need for "NB:".

@lukaskubanek

Copy link
Copy Markdown
Contributor Author

@mbrandonw Applied, thanks for the pointer about comments above #expect ending up in failure messages. I remember seeing this behavior in Xcode but I wasn’t aware of it being officially supported.

I also merged main into the branch. There was a conflict in Metadatabase.swift with the configuration passing introduced in #502, which I resolved by handing the configuration to both the DatabaseQueue and DatabasePool branches.

PR should be good to go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants